Add optional --ssl-ca-bundle flag for SSL certificate configuration#1802
Merged
haritamar merged 9 commits intoelementary-data:masterfrom Mar 2, 2026
Merged
Add optional --ssl-ca-bundle flag for SSL certificate configuration#1802haritamar merged 9 commits intoelementary-data:masterfrom
haritamar merged 9 commits intoelementary-data:masterfrom
Conversation
szaffarano
force-pushed
the
szaffarano/ssl-ca
branch
from
81fddaa to
746ce80
Compare
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... 📒 Files selected for processing (4)
✏️ Tip: You can disable in-progress messages and the fortune message in your review settings. 📝 WalkthroughWalkthroughAdds SSL CA bundle configuration and flows: a new Config.ssl_ca_bundle value and create_ssl_context utility; threads an Optional[ssl.SSLContext] through Slack client construction and Slack messaging integrations; exposes a CLI option --ssl-ca-bundle to pass the CA bundle into Config and downstream components. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI (--ssl-ca-bundle)
participant Config as Config
participant Integrations as Alerts/Integrations
participant SSLUtil as create_ssl_context
participant SlackClient as SlackClient / Integration
participant SlackSDK as Slack Webhook/WebClient
CLI->>Config: construct Config(ssl_ca_bundle)
Config->>Integrations: provide config
Integrations->>SSLUtil: create_ssl_context(config.ssl_ca_bundle)
SSLUtil-->>Integrations: ssl_context or None
Integrations->>SlackClient: initialize(..., ssl_context)
SlackClient->>SlackSDK: instantiate WebClient/WebhookClient(ssl=ssl_context)
SlackSDK-->>SlackClient: client ready
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
elementary/clients/slack/client.py(5 hunks)elementary/config/config.py(2 hunks)elementary/monitor/cli.py(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
elementary/clients/slack/client.py (1)
elementary/monitor/data_monitoring/alerts/integrations/slack/slack.py (1)
_initial_client(87-93)
Contributor
|
This pull request is stale because it has been open for too long with no activity. |
haritamar
force-pushed
the
szaffarano/ssl-ca
branch
from
5e7da25 to
572e790
Compare
haritamar
temporarily deployed
to
elementary_test_env
GitHub Actions
Inactive
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@elementary/clients/slack/client.py`:
- Around line 263-271: The _initial_client method currently returns a bare
requests.Session() when self.is_workflow is True, which ignores the ssl_context;
update _initial_client so that when is_workflow is True you either configure the
returned requests.Session to honor ssl_context (e.g., set session.verify to the
provided SSLContext or CA bundle path, or attach a TransportAdapter that uses
it) or emit a clear warning via the existing logger explaining that
--use-system-ca-files/ssl_context will be used/ignored for workflow webhooks;
reference _initial_client, self.is_workflow, ssl_context, requests.Session, and
WebhookClient to locate and change the behavior.
- Update _initial_client in SlackWebhookClient to configure requests.Session with SSL verification - When ssl_context is provided, set session.verify to certifi CA bundle - Add warning when workflow webhooks cannot fully honor --use-system-ca-files setting - Ensures SSL context is respected for workflow webhook requests
haritamar
temporarily deployed
to
elementary_test_env
GitHub Actions
Inactive
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@elementary/clients/slack/client.py`:
- Around line 264-276: The warning and comment around requests.Session SSL
handling are incorrect and the use_system_ca_files flag isn't honored; update
the workflow webhook branch in the method that references is_workflow and
ssl_context so that when ssl_context is None (meaning use_system_ca_files=True)
you explicitly set session.verify to the OS trust store path (or load OS CAs via
a library like truststore) and only log a warning if you fail and must fall back
to certifi.where(); when ssl_context is not None set session.verify =
certifi.where() and change the warning text to reflect that certifi is being
used (not that it's overriding a system-ca preference). Ensure the comment is
corrected to state requests.Session defaults to certifi.where() and document
which branch uses the OS CA file vs certifi.
Replace the boolean --use-system-ca-files/--no-use-system-ca-files flag with a more flexible --ssl-ca-bundle option that accepts 'certifi', 'system', or a custom file path. When omitted, each library keeps its own default CA behaviour (no change from prior behaviour). Key changes: - Add elementary/utils/ssl.py helper to resolve ssl_ca_bundle into SSLContext - Config now uses _first_not_none pattern so ssl_ca_bundle is also loadable from config.yml - Apply SSL context to both legacy SlackClient and newer SlackWebMessagingIntegration / SlackWebhookMessagingIntegration code paths Made-with: Cursor
haritamar
requested a deployment
to
elementary_test_env
GitHub Actions
Waiting
haritamar
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
elementary/monitor/data_monitoring/alerts/integrations/integrations.py (1)
47-52: Delay SSL context creation until after the workflow short-circuit.
ssl_contextis resolved before the workflow return path but is unused there. Moving creation belowif config.is_slack_workflowavoids unnecessary work in this branch.Proposed refactor
if config.has_slack: - ssl_context = create_ssl_context(config.ssl_ca_bundle) if config.is_slack_workflow: return SlackIntegration( config=config, tracking=tracking, ) + ssl_context = create_ssl_context(config.ssl_ca_bundle) if config.slack_token: return SlackWebMessagingIntegration.from_token( config.slack_token, tracking, ssl_context=ssl_context )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@elementary/monitor/data_monitoring/alerts/integrations/integrations.py` around lines 47 - 52, The code currently calls create_ssl_context(config.ssl_ca_bundle) before checking config.is_slack_workflow, causing unnecessary work when the function returns a SlackIntegration; move the create_ssl_context call so it only runs when not short-circuited by the Slack workflow path: first check if config.is_slack_workflow and return SlackIntegration(config=config, tracking=tracking) immediately, and only after that create_ssl_context(config.ssl_ca_bundle) and use it for the other integration construction paths.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@elementary/utils/ssl.py`:
- Around line 24-25: The code currently does value = ssl_ca_bundle.strip() and
then proceeds to file-path checks which will raise a misleading file error for
blank inputs; modify the logic around the variable value (derived from
ssl_ca_bundle) to add an explicit branch that treats an empty or all-whitespace
value as a validation error (e.g., raise a clear ValueError or return a clear
validation result) before any filesystem/path existence checks occur (the checks
that use value later); update both occurrences that perform strip+file checks so
blank inputs are validated early and produce a specific, descriptive error
instead of a file-not-found error.
---
Nitpick comments:
In `@elementary/monitor/data_monitoring/alerts/integrations/integrations.py`:
- Around line 47-52: The code currently calls
create_ssl_context(config.ssl_ca_bundle) before checking
config.is_slack_workflow, causing unnecessary work when the function returns a
SlackIntegration; move the create_ssl_context call so it only runs when not
short-circuited by the Slack workflow path: first check if
config.is_slack_workflow and return SlackIntegration(config=config,
tracking=tracking) immediately, and only after that
create_ssl_context(config.ssl_ca_bundle) and use it for the other integration
construction paths.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (7)
elementary/clients/slack/client.pyelementary/config/config.pyelementary/messages/messaging_integrations/slack_web.pyelementary/messages/messaging_integrations/slack_webhook.pyelementary/monitor/cli.pyelementary/monitor/data_monitoring/alerts/integrations/integrations.pyelementary/utils/ssl.py
🚧 Files skipped from review as they are similar to previous changes (1)
- elementary/config/config.py
Raise a clear error when ssl_ca_bundle is an empty or whitespace-only string, instead of falling through to the file path check which would produce a confusing "path does not exist" error. Made-with: Cursor
haritamar
requested a deployment
to
elementary_test_env
GitHub Actions
Waiting
Keep both `import ssl` (from our branch) and `timezone` import (from master). Made-with: Cursor
haritamar
temporarily deployed
to
elementary_test_env
GitHub Actions
Inactive
haritamar
requested a deployment
to
elementary_test_env
GitHub Actions
Waiting
haritamar
approved these changes
Mar 2, 2026
Collaborator
|
Thanks so much @szaffarano for this contribution! I changed it to |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Addresses #1801 by adding a new optional
--ssl-ca-bundleCLI flag that lets users control which CA certificates are used for SSL connections (e.g. to Slack).Problem
Users in corporate environments or minimal containers may need to override the default CA bundle used for SSL connections. Previously there was no way to control this.
Solution
A new
--ssl-ca-bundleoption is added to themonitorandsend-reportcommands. It accepts:certifi— use the certifi package's Mozilla CA bundlesystem— use the OS system CA store/etc/ssl/certs/my-corp-ca.pem)When omitted (the default), each underlying library keeps its own default CA behaviour — no change from prior behaviour.
The setting can also be persisted in
config.ymlasssl_ca_bundle.Key changes
elementary/utils/ssl.py(new) — helper that resolves thessl_ca_bundlevalue into anssl.SSLContextelementary/config/config.py— newssl_ca_bundleconfig option, loadable from both CLI andconfig.ymlelementary/monitor/cli.py—--ssl-ca-bundleClick option onmonitorandsend-reportelementary/clients/slack/client.py— legacySlackClientpasses the resolved SSL context toWebClient/WebhookClientelementary/messages/messaging_integrations/slack_web.py—from_tokenaccepts an optional SSL contextelementary/messages/messaging_integrations/slack_webhook.py—from_urlaccepts an optional SSL contextelementary/monitor/data_monitoring/alerts/integrations/integrations.py— resolves and passes SSL context to both newer messaging integration code pathsNotes
requests.Session()which is architecturally bound to certifi. The--ssl-ca-bundleflag does not affect this path — it always uses certifi's bundle regardless of the setting.Summary by CodeRabbit
New Features
--ssl-ca-bundleCLI option or configuration file.